-
Notifications
You must be signed in to change notification settings - Fork 3
Implement ElectricMultipoleParameters #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
6d57eb5 to
3313a3a
Compare
|
I was looking into duplicating most of the Is it expected that the pals-python/src/pals/kinds/Quadrupole.py Lines 7 to 15 in 2ddf2a7
Shouldn't a quadrupole be either electric or magnetic? |
|
@EZoni Here is my point of view: By far the most common usage of "Quadrupole" refers to a magnetic quadrupole, for which all the electric multipole contributions vanish. However, there has been a general trend to allow optional contributions from other parameter groups where reasonable (for example, to support different types of "combined-function" elements). There are also pure electric quadrupoles, for which the same situation applies in reverse (although these are less common). If we wanted to treat electric and magnetic quadrupoles in a symmetric way, there would be two options: 1) create a separate ElectricQuadrupole kind; or 2) require the specification of either magnetic or electric quadrupole components or both (e.g., at least one must be specified). Either option appears reasonable to me. |
|
Thanks, @cemitch99! The standard, https://pals-project.readthedocs.io/en/latest/element-kinds.html#quadrupole-element, seems to go more in the direction of your option 2, "require the specification of either magnetic or electric quadrupole components or both (e.g., at least one must be specified)", since it doesn't mention any I think I will propose a change that implements option 2 and see if it works for everyone. |
|
@EZoni That sounds good; I agree it's a simple and clean option. |
|
Do you think the same is true for Right now they seem to allow for no parameter at all, neither MagneticMultipoleP nor ElectricMultipoleP, i.e., both are optional. For example, we have this
without extra validation logic that requires at least one of the two to be present (like the logic I added in this PR for I could fix this in a separate PR, once the "under construction" flag is removed. Do you think this does need a fix indeed? |
| MagneticMultipoleP: MagneticMultipoleParameters | ||
|
|
||
| @model_validator(mode="after") | ||
| def validate_at_least_one_multipole(self) -> "Quadrupole": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude Haiku 4.5 through Visual Studio Code thinks that it is appropriate to include the "Quadrupole" type annotation here:
The string
"Quadrupole"is just a type annotation - it doesn't affect what the method actually returns at runtime. The method still returnsself(the actual instance object), which is what Pydantic needs. The-> "Quadrupole"is only metadata for type checkers and IDE tools; it doesn't change the runtime behavior at all. The quotes around"Quadrupole"are used because:
- The class is still being defined, so we can't reference
Quadrupoledirectly yet (forward reference).- It's equivalent to writing
-> Quadrupoleafter the class definition is complete.So to be clear: at runtime, the method returns the
selfobject (the actualQuadrupoleinstance), not a string. The type annotation is purely for static analysis.
|
@EZoni I plan to contribute Sextupole, Octupole, and Multipole sections to the standard shortly, since they are simple and widely used. I don't see any good argument for allowing these element kinds with no multipole components at all (since then they have no reason to be classified this way), so adding a validation doesn't hurt. In some ways, it is a bare minimum sanity check. |
Close #38.
To do:
ElectricMultipoleParameters.